BP5: thread-safe context-bearing Get/PerformGets pipeline#5050
Merged
Conversation
pnorbert
previously approved these changes
Jun 1, 2026
eisenhauer
force-pushed
the
bp5-get-context
branch
2 times, most recently
from
3845045 to
11e3234
Compare
NewGetContext returns a per-caller GetContext; Get(ctx, var, data, sel)
and PerformGets(ctx) operate on independent queues, so concurrent reads
on one engine no longer race. NewGetContext returns nullptr unless the
engine is ReadRandomAccess on a reentrant Read transport (POSIX) — the
default acts as a feature probe.
Core:
* core::GetContext polymorphic base.
* Engine::NewGetContext (virtual, nullptr default), PerformGets(ctx),
Get(ctx, ...) template, DoGetContextDeferred typeless virtual.
* Same surface exposed through the CXX binding.
BP5:
* BP5GetContext (nested in BP5Deserializer) owns what was
BP5Deserializer::PendingGetRequests.
* QueueGet, GenerateReadRequests, FinalizeGet/Gets/DerivedGets gain
ctx-bearing forms; legacy forms forward via m_DefaultGetContext.
* BP5Reader overrides NewGetContext (guarded on ReadRandomAccess +
cached m_HasReentrantReadTransport), PerformGets(ctx) (local only —
remote throws), DoGetContextDeferred.
XRootD plugin:
* SubPool dual mode: first Open probes NewGetContext; if non-null,
one engine is shared across concurrent users (in_use_count =
refcount); otherwise legacy per-user exclusive borrow.
* Single-get and batch handlers build per-request Selection inline
(no Variable mutation) and use the ctx form when supported, with
a single PerformGets(ctx) per batch for coalescing.
Tests (testing/adios2/engine/bp/TestBPGetContextIsolation):
* TwoIndependentContexts: PerformGets(ctxA) fills only ctxA, leaves
ctxB at sentinel; context reusable; empty-queue PerformGets no-op.
* ConcurrentGetsOnSameEngine: TSan-validated.
* NonReentrantTransportRejected: fstream transport => NewGetContext
returns nullptr.
eisenhauer
force-pushed
the
bp5-get-context
branch
from
11e3234 to
b394f8c
Compare
pnorbert
approved these changes
Jun 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the final bit of a long arc towards thread-safe Get() operations, mostly to benefit the XRootD ADIOS server. This takes advantage of the special case where we're using a POSIX transport (with using pread(), so no kernel-level state in the sockets, currently the default), and ReadRandomAccess where the metadata is pulled in at Open() and remains unchanging thereafter. This opened up the possibility for thread-safe Get() operations, as long as you could specify the selection without modifying Variable state (enabled by the previous SelectionStruct additions), and as long as the PendingGetRequests queue was abstracted into something that could be specified separately from the engine (these changes).
This PR includes these changes as well as the mods to the XRootD file pool that take advantage of them (allow multiple threads to reuse an open file, as opposed to only reusing open files when there is no current operation on it).
While this feature is long-planned, the introduction of batch Get() operations and other optimizations at the curl https layer have significantly reduced the need for it. For example, in the KSTAR data used in campaign testing, all the fetches for a particular file land in a single Get() batch, so there are never concurrent requests for the same file (and so no benefit to this change). But other scenarios, particularly near-simultaneous fetches from multi-rank executables (ML training?) should show benefits from this.
This does make sure that batch Gets() are done deferred, so that multiple data reading threads can fetch the data. This was an oversight in prior work that we're fixing while we're modifying that particular code to use the new ReadContext.